Repository navigation
feat(hook-manager): add step-hook-manager-shim eval suite for Step P.3b (part of #1891) - #1892
Conversation
github-actions
Bot
added
family:setup
Kaap10
changed the title
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
The suite is well shaped and all 7 cases pass on the head, but its output spec restates Step P.3b's shim rule and the expected answers, so it would keep passing if the prose it is meant to cover regressed.
evals catch a skill that silently starts producing different output (tools/skill-evals/evals/setup-isolated-setup-install/step-hook-manager-shim/fixtures/output-spec.md:18)
Both signals catch the same class of regression: a skill that silently starts producing different output (eval failure) or that silently became materially more expensive to run (cost-table drift).
- Per Step P.3b item 2:
- Copy `tools/agent-isolation/hook-manager-shim.sh` into `~/.claude/bin/<tool>` once for each adapter in the hook-manager registry with an install strategy other than `none` that a repository the operator works in uses
The runner appends output-spec.md to the extracted Step P.3b section (load_step_config, runner.py:199-202), so these lines put the rule under test into the prompt a second time, and line 21 gives the answer for every none manager by name. If item 2 of Step P.3b lost its strategy filter, the suite would still pass on this copy, which is the regression #1838 asked this suite to catch. I ran the suite on the head with claude -p and all 7 cases pass, but that cannot tell the two sources apart.
The model does need to know which managers are none, since Step P.3b points at the registry rather than naming them. Could you include the registry instead ("also_include": ["tools/hook-manager/registry.json"] in step-config.json) and cut this file down to the two field definitions and the JSON-only rule? Then a check that the suite fails with the strategy sentence removed from item 2 would show it covers the prose.
Flagged by both the primary and adversarial reviewers.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. The findings
below are observations, not blockers; an Apache Magpie
maintainer — a real person — will take the next look at the
PR. If you think a finding is mis-applied, please reply on
the PR and a maintainer will weigh in.More on how Apache Magpie handles maintainer review:
Contributing guide.
…her than output-spec
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
The new head does what the earlier review asked: the registry now comes in through also_include, and output-spec.md is down to the field definitions and the JSON-only rule, so the suite reads Step P.3b's own prose against the live registry. The counts (28 cases across 6 steps) and the fixtures match the registry.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. The maintainer
approving this PR has read the findings and signed off. If
something feels off, please reply on the PR and a maintainer
will follow up.More on how Apache Magpie handles maintainer review:
Contributing guide.
Summary
step-hook-manager-shim(7 cases) undertools/skill-evals/evals/setup-isolated-setup-install/for Step P.3b (### Step P.3b-whole-user — Install the per-repo dispatcher + hook-manager shim (dispatcher flavour only)).install.strategy: "none"(simple-git-hooks,cargo-husky,git-code-format-maven-plugin,git-build-hook) and installs the shim exclusively for managers requiring an install fix (prek).tools/skill-evals/README.mdandtools/skill-evals/evals/setup-isolated-setup-install/README.md.Type of change
.claude/skills/<name>/) — eval fixtures updated belowtools/<system>/*.md)tools/*/withpyproject.toml)docs/,README.md,CONTRIBUTING.md)projects/_template/)prek, workflows, validators)Test plan
prek run --all-filespassesuv run pytest/ruff check/mypypasses(
PYTHONPATH=tools/skill-evals/src python3 -m skill_evals.runner tools/skill-evals/evals/<skill>/)(a regression test for the bug fixed / the behaviour added — see CONTRIBUTING.md)
sync-hook-managers.py --checkpasses cleanly (0 drift);test_sync_hook_managers.pypasses (25/25); markdownlint passes (42 files, 0 issues); all 7 cases load and self-compare cleanly in runner.RFC-AI-0004 compliance
<PROJECT>,<tracker>,<upstream>,<security-list>) used in all skill / tool prose (thecheck-placeholdersprek hook is the mechanical gate)Linked issues
Part of #1891 (Part 1 of 2)
Refs #1838
Notes for reviewers (optional)
Follow-up Part 1 from PR #1838 merge feedback:
The suite tests 7 concrete scenarios:
case-1-prek-repo: whole-user dispatcher flavour, repo withprek.toml->installed_shims: ["prek"]case-2-simple-git-hooks-repo: whole-user dispatcher flavour, repo withsimple-git-hooks->installed_shims: []case-3-cargo-husky-repo: whole-user dispatcher flavour, repo with.git/hooks/pre-pushhook marker ->installed_shims: []case-4-maven-repo: whole-user dispatcher flavour, repo withpom.xml(git-code-format-maven-plugin+git-build-hook) ->installed_shims: []case-5-mixed-repos: whole-user dispatcher flavour, mixed repos (prek + simple-git-hooks + maven) ->installed_shims: ["prek"]case-6-unmanaged-repo: whole-user dispatcher flavour, standard git repo without hook managers ->installed_shims: []case-7-non-dispatcher-scope: operator pickedper-projectscope ->dispatcher_installed: false,installed_shims: []Part 2 (live empirical execution of the two Maven plugins under dispatcher) will follow in a separate PR when a live Maven runner environment is available.